The logic here is almost same as getPartition() in DescTableOperation.java https://github.com/apache/hive/pull/6259/changes#diff-641c62b42b01bff41c89a3b3661c15d6c08fce0e48740347f36fe32448984147R131

Check if you can add them in an utility so it can be reused

Thanks for checking it, moved out this logic from DescTableOperation.java and DescTableAnalyzer.java to HiveIcebergStorageHandler.java
https://github.com/apache/hive/pull/6259/changes#diff-93864ecf035fe51b92185015da842a56837cea89064813de39c278c6f8fed03cR2079
please take a look

Please don't use DDLUtils.isIcebergTable as this API in the compiler is too specific. Instead please use tab.isNonNative() in conjunction to other APIs if needed.

Thanks for checking it, tried to make changes more generic to non native tables instead of iceberg centric please take a look

@ramitg254, consider joining with Table.hasNonNativePartitionSupport

drop this

actually I added it because of these considerations:

1. getPartition in iceberg returns dummy partition corresponding to that partSpec without checking it is already present or not and this functionality is used in other places like for insert queries for iceberg tables as well that's why I added this separate method

2. I also thought of adding this method in Hive.java instead of storage handler but that would be too specific and if we have newer storage handler in future then it can have some different implementation to check the partition is present or not

getPartition in iceberg returns dummy partition corresponding to that partSpec without checking it is already present or not

exactly. i think we need to change that API and also drop the RewritePolicy policy arg (move Context.RewritePolicy.get(conf) inside the StorageHandler).
getPartition() should return dummy partition corresponding to the provided partSpec only if exists otherwise it confusing.

please check if ATM this method is only used by Iceberg compaction. in that scenario we could skip the validation (i.e. check for SessionState.get().isCompaction())
cc @difin

@deniskuzZ updated the getPartition api implementation as per the suggestion and tests are passing without affecting any insertion and compaction related queries. Please have a look

LGTM, but I have a question for @difin regarding whether isPartitionPresent(table, partitionSpec) is necessary in the context of compaction?

do we really need
Warehouse.makePartName(partitionSpec, false).equals(partNames.getFirst())
check?
isn't !partNames.isEmpty() enough? we already pass the partitionSpec to getPartitionNames

But let's say for eg. for partspec is c=6 and parts available are c=6/d=hello6 only, then we would like to throw partition not found exception and that's why .equals is required as well

getTable() could be moved out of try block. i think same is true for hasPartition check

not sure, but maybe we could move hasNonNativePartitionSupport logic inside of Table#getPartCols() , similar to

  public List<String> getPartColNames() {
    List<FieldSchema> partCols = hasNonNativePartitionSupport() ?
        getStorageHandler().getPartitionKeys(this) : getPartCols();
    return partCols.stream().map(FieldSchema::getName)
      .collect(Collectors.toList());
  }

same here